[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwhonce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

@mheon https://github.com/containers/podman/pull/11103/files#diff-02a7031ca65fa34d69f3de533b25111aa07a03242c86c4660dbd35ec69d1cd2fR137 is causing me heartburn. Without a containers.Detach() it is not clear to me when the response.Body can be closed... /cc @baude

@mheon https://github.com/containers/podman/pull/11103/files#diff-02a7031ca65fa34d69f3de533b25111aa07a03242c86c4660dbd35ec69d1cd2fR137 is causing me heartburn. Without a containers.Detach() it is not clear to me when the response.Body can be closed... /cc @baude

I think Attach() blocks until it reads EOF. So defer should work no?

@mheon https://github.com/containers/podman/pull/11103/files#diff-02a7031ca65fa34d69f3de533b25111aa07a03242c86c4660dbd35ec69d1cd2fR137 is causing me heartburn. Without a containers.Detach() it is not clear to me when the response.Body can be closed... /cc @baude

I think Attach() blocks until it reads EOF. So defer should work no?

@Luap99 I thought so, but the client fails to get any output when I defer the Close(). I have not had time to debug it yet.

@jwhonce Once Attach has handed over to Libpod, all responsibility for closing the connection is handed over to that code (the termination conditions are very complex, and the only place that has enough information to be able to know when they are met is Libpod). As such, that defer will never actually close the body, because it is guaranteed to already be closed by the end of the function.

Can you describe what's going on / why the body needs to be closed in more detail?

@mheon If we don't close body at the end of the operation we "leak" the file descriptor. From godoc

The client must close the response body when finished with it:

resp, err := http.Get("http://example.com/")
if err != nil {
	// handle error
}
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
// ...

@jwhonce I think the key is that we can only do that if the connection has not been hijacked (hijackChan in the handler indicates that this has happened). Once hijacking is complete, all responsibility for closing goes to the Libpod HTTPAttach function.

/lgtm